Skip to content

Conversation

mati865
Copy link
Member

@mati865 mati865 commented Aug 11, 2020

Just like other #![plugin(...)] tests it requires stage2 to work correctly.

Just like other `#![plugin(...)]` tests it requires stage2 to work correctly.
@rust-highfive
Copy link
Contributor

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2020
@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 11, 2020

What is the point of running ui-fulldeps tests on stage 1?
All of them are supposed to not pass. (The passing ones, if any, should be moved ui.)

@mati865
Copy link
Member Author

mati865 commented Aug 11, 2020

I think since #73964 running ./x.py test will use stage1 for ui-fulldeps and this is the only error I have noticed.
cc @jyn514

@petrochenkov
Copy link
Contributor

Interesting. I never tried to run full x.py test on stage 1.

this is the only error I have noticed

I also noticed that x.py test src/tools/clippy fails (despite building the compiler twice) and needs specifying --stage 2 explicitly, but it's not run by x.py test by default.

@jyn514
Copy link
Member

jyn514 commented Aug 11, 2020

IIUC this was always broken with --stage 1 but nobody noticed because it wasn't the default? This fix seems fine to me then.

@petrochenkov
Copy link
Contributor

This is not fine because x.py test <component> is supposed to test the component.
This was true previously, but not true now.
The whole default stage change caused more confusion than benefit, IMO.

@mati865
Copy link
Member Author

mati865 commented Aug 12, 2020

Like half of the ui-fulldeps tests is ignored in stage1 so the issue is much more than this single test.

@jyn514
Copy link
Member

jyn514 commented Aug 12, 2020

This is not fine because x.py test <component> is supposed to test the component.
This was true previously, but not true now.
The whole default stage change caused more confusion than benefit, IMO.

I wish you'd raised this concern during the MCP :/

AFAIK x.py test ui-fulldeps will show that the tests are being ignored, which might push people towards --stage 2. Is that clear enough or do you think it would be helpful to print a warning from x.py as well? Something like 'x.py test --stage 1 ui-fulldeps inherently cannot work; try using --stage 2 instead'.

@Mark-Simulacrum
Copy link
Member

If we truly need stage 2, then it might make sense to just error in stage 1. I don't know that it's a hard requirement that test suites pass in stage 1. We could annotate them with "needs-stage2" or so such that you can get a nicer error message (e.g., in stage 1 we just list failures in those tests rather than actually trying to run them).

@petrochenkov
Copy link
Contributor

Yeah, if stage 1 stays a default, then we should probably

  • error on x.py test <suite>/x.py test --stage 1 <suite> if the test suite requiring stage 2 is specified explicitly,
  • ignore it with a message in "glob" runs like x.py test without arguments.

@petrochenkov
Copy link
Contributor

(Also, I'm pretty sure at least some tests are in *-fulldeps directories accidentally and can be moved to stage 1 test suites.)

@mati865
Copy link
Member Author

mati865 commented Aug 12, 2020

@petrochenkov and remove ignore-stage1 from already existing ui-fulldeps tests.

I'm going to close this PR since it's not going to be accepted.

@mati865
Copy link
Member Author

mati865 commented Aug 25, 2020

Opened #75905

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants